ci: add a github bot to support advanced PR review workflows #3037

aeddi · 2024-10-28T10:00:13Z

This pull request aims to add a bot that extends GitHub's functionalities like codeowners file and other merge protection mechanisms. Interaction with the bot is done via a comment. You can test it on the demo repo here : GnoCheckBot/demo#1

Fixes #1007
Related to #1466, #2788

The config.go file contains all the conditions and requirements in an 'If - Then' format.

// Automatic check

{
  Description: "Changes to 'tm2' folder should be reviewed/authored by at least one member of both EU and US teams",
  If: c.And(
    c.FileChanged(gh, "tm2"),
    c.BaseBranch("main"),
  ),
  Then: r.And(
    r.Or(
      r.ReviewByTeamMembers(gh, "eu", 1),
      r.AuthorInTeam(gh, "eu"),
    ),
    r.Or(
      r.ReviewByTeamMembers(gh, "us", 1),
      r.AuthorInTeam(gh, "us"),
    ),
  ),

}

There are two types of checks: some are automatic and managed by the bot (like the one above), while others are manual and need to be verified by a specific org team member (like the one below). If no team is specified, anyone with comment editing permission can check it.

// Manual check
{
  Description: "The documentation is accurate and relevant",
  If:          c.FileChanged(gh, `.*\.md`),
  Teams: []string{
    "tech-staff",
    "devrels",
  },
},

The conditions (If) allow checking, among other things, who the author is, who is assigned, what labels are applied, the modified files, etc. The list is available in the condition folder.
The requirements (Then) allow, among other things, assigning a member, verifying that a review is done by a specific user, applying a label, etc. (List in requirement folder).
A PR Check (the icon at the bottom with all the CI checks) will remain orange/pending until all checks are validated, after which it will turn green.

The Github Actions workflow associated with the bot ensures that PRs are processed concurrently, while ensuring that the same PR is not processed by two runners at the same time.
We can manually process a PR by launching the workflow directly from the GitHub Actions interface.

To do

implement base version of the bot
cleanup code / comments
setup a demo repo
add debug printing on dry run
add some tests on requirements and conditions

Contributors' checklist...

Added new tests, or not needed, or not feasible
Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
Updated the official documentation or not needed
No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
Added references to related issues and PRs
Provided any useful hints for running manual tests

codecov · 2024-10-28T10:02:04Z

Codecov Report

Attention: Patch coverage is 60.94840% with 280 lines in your changes missing coverage. Please review.

Project coverage is 63.79%. Comparing base (4f27a57) to head (b12be4d).
Report is 9 commits behind head on master.

Files with missing lines	Patch %	Lines
contribs/github-bot/client/client.go	35.32%	116 Missing and 3 partials ⚠️
contribs/github-bot/param/param.go	0.00%	70 Missing ⚠️
contribs/github-bot/logger/terminal.go	0.00%	23 Missing ⚠️
contribs/github-bot/param/prlist.go	0.00%	18 Missing ⚠️
contribs/github-bot/logger/action.go	0.00%	14 Missing ⚠️
contribs/github-bot/requirement/branch.go	69.23%	6 Missing and 2 partials ⚠️
contribs/github-bot/requirement/reviewer.go	91.78%	4 Missing and 2 partials ⚠️
contribs/github-bot/logger/logger.go	28.57%	5 Missing ⚠️
contribs/github-bot/logger/noop.go	37.50%	5 Missing ⚠️
contribs/github-bot/condition/file.go	85.18%	3 Missing and 1 partial ⚠️
... and 2 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3037      +/-   ##
==========================================
+ Coverage   63.77%   63.79%   +0.01%     
==========================================
  Files         548      573      +25     
  Lines       78681    79587     +906     
==========================================
+ Hits        50180    50773     +593     
- Misses      25117    25407     +290     
- Partials     3384     3407      +23

Flag	Coverage Δ
contribs/github-bot	`60.94% <60.94%> (?)`
contribs/gnodev	`60.54% <ø> (ø)`
contribs/gnofaucet	`14.82% <ø> (ø)`
gno.land	`73.70% <ø> (+0.07%)`	⬆️
gnovm	`67.93% <ø> (+<0.01%)`	⬆️
misc/genstd	`79.72% <ø> (ø)`
tm2	`62.49% <ø> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

thehowl · 2024-10-30T20:52:01Z

@aeddi would you like a preliminary review?

ltzmaxwell · 2024-11-05T03:05:56Z

could you please add a brief description to give people an early preview, even if it's draft?

aeddi · 2024-11-05T17:52:38Z

@thehowl Sorry for the delayed response. I hadn't seen your message. I still have some cleanup to do and need to figure out how to write some tests, but I think the PR is pre-reviewable now.
Thanks!

@ltzmaxwell Done ;)

ltzmaxwell · 2024-11-06T02:03:14Z

I haven't looked into this in depth yet, but I'm considering whether we could introduce an LLM, like ChatGPT, to assist with code review. It could help summarize PR modifications and potentially identify issues in the code. This is outside the current scope, but I wanted to leave some thoughts here for discussion.

aeddi · 2024-11-06T07:48:24Z

Yes, why not have this kind of tool just to detect details we might have missed?
I know it's a controversial topic, but personally, knowing that our code is completely open-source, I wouldn't mind having a report from an LLM to pick out only the relevant suggestions.

Just quickly checking, I see there are Actions on the Github marketplace dedicated to this.

moul · 2024-11-12T16:30:05Z

When do you think we should start using it and providing feedback for iteration?

aeddi · 2024-11-12T16:54:24Z

@moul As soon as it's reviewed and merged, I think we can start by setting up a few simple rules to make sure everything is working well, then iterate from there.
The problem is that it's tricky to test a bot based on GitHub Actions. I've done a lot of testing on two private repos, but I hope there aren't any issues I haven't anticipated.
For example, we can test safe operations like requesting reviews from certain users + adding a manual check, or something like this.

BTW, the failing check is because the bot workflow added to this PR is trying to run while the repo isn't configured yet (specifically, the workflow requires setting a GitHub Actions secret and variable).

ajnavarro

I think this way of implementing a bot is not secure. Giving random access to code execution to anyone creating a PR is not a good idea IMO.

ajnavarro · 2024-11-13T10:56:28Z

.github/workflows/bot.yml

+      - assigned
+      - unassigned
+      - labeled
+      - unlabeled
+      - opened
+      - reopened
+      - synchronize # PR head updated
+      - converted_to_draft
+      - ready_for_review


All existing types for reference:

- assigned - unassigned - labeled - unlabeled - opened - edited - closed - reopened - synchronize - converted_to_draft - locked - unlocked - enqueued - dequeued - milestoned - demilestoned - ready_for_review - review_requested - review_request_removed - auto_merge_enabled - auto_merge_disabled

Why don't just run on all types?

I only included the ones that were used by the existing conditions/requirements of the bot. It seems better to me than subscribing to all possible events for several reasons :

It makes it clearer which events the bot uses

in term of security, it follows the PoLP (even though a trigger is not exactly a privilege)

it prevents the workflow being triggered and launching two runners for nothing

With the idea that if we added, for example, a condition/requirement based on adding to/removing from a milestone, we would just add the corresponding trigger event to the workflow.

ajnavarro · 2024-11-13T10:56:56Z

.github/workflows/bot.yml

+
+  # Watch for changes on PR comment
+  issue_comment:
+    types: [created, edited, deleted]


The same, why don't just run on all types and let the bot decide what to do?

Same as above.

ajnavarro · 2024-11-13T11:00:35Z

.github/workflows/bot.yml

+          go-version-file: go.mod
+
+      - name: Run GitHub Bot
+        working-directory: contribs/github-bot


More than a bot is a script, right? I was expecting a server handling events through the GitHub API with a state.

When we defined the specifications with @moul and @thehowl (I can't remember who else was on the call we had in Turin about it), one of the requirements was that the bot shouldn't run on a server but that everything should be processed through GitHub Actions.

It makes things more complicated to test and the manual checks more complicated to implement using the comment body as their state instead of a DB or similar, but it has the advantage of having a piece of code that runs in a serverless way.

But if the name is misleading, we can rename it.

ajnavarro · 2024-11-13T11:01:56Z

.github/workflows/bot.yml

+  # This job creates a matrix of PR numbers based on the inputs from the various
+  # events that can trigger this workflow so that the process-pr job below can
+  # handle the parallel processing of the pull-requests
+  define-prs-matrix:


I don't fully understand this job. Do you mind to explain it?

Yes sorry if it's not clear enough, so there are two constraints:

First, we need to prevent multiple runners from processing the same PR at the same time (to avoid any bug)

Second, there are different events that can trigger this workflow, and not all of them provide the PR number(s) to process the same way.

So the goal of this job (define-prs-matrix) is first to establish a list of PR numbers to process from the different possible events, then to format this list as a matrix so that the next job (process-pr) can launch a runner for each PR number in the matrix while ensuring (through the 'concurrency' feature of GitHub Actions) that no PR is processed concurrently.

@aeddi some additional questions here:

Is there a scenario where one PR can block another PR? Or concurrency only happen when manually triggered?

How much time does a job usually take to complete? If one PR can block another PR and take 10 minutes (e.g., long process, network issue, ...) to complete, it might start to be a problem.

The concurrency parameter of the job is really set to prevent a PR X from being processed by two runners in parallel. For example, if PR 1111 is already being processed by one runner and a new event (any event, manual or not) triggers the job to also process PR 1111, the new runner will be queued and will wait for the first runner to finish execution before starting the job. However, the processing of PR 2222 can occur without any issues in parallel with that of 1111.

Never saw a run last longer than 1 min, on average the execution time is around 40 secs (and one PR can't block another one anyway ;) )

aeddi · 2024-11-13T17:39:37Z

I'm not sur to understand what differ from any other action. I mean to run code when opening a PR or any other GitHub event, we already have more than 130 checks that run for each PR we open.
But maybe I didn't fully understand what you meant?

Also in the case of this bot, we own its code in our repo and have total control over it. Which looks more secure to me than using third-party actions.

Still following the PoLP, the only permissions the bot requires right now are those listed in the README, which are not very sensitive IMO. The write permissions are for the scopes Pull Requests (managing comments, labels, milestones, etc.) and Commit Statuses (showing a check at the bottom of the PR). It can't change any code on the repo, for instance.

Do you have another approach to suggest or specific guideline to follow?

gfanton

overall LGTM, my only requirement is to get ride of this // nolint:govet and fix cancel leaking.

I'm not sure about @ajnavarro's concern; as long as the token is correctly scoped, it should be fine IMO.

To me, the main issue is that if the PR comes from a fork, the workflow could not be able to grab the bot secret and therefore cannot access the token. Can you (have you?) check the scenario where you trigger this workflow from a branch of an external fork?

from: https://docs.github.com/en/actions/security-for-github-actions/security-guides/using-secrets-in-github-actions#using-secrets-in-a-workflow

With the exception of GITHUB_TOKEN, secrets are not passed to the runner when a workflow is triggered from a forked repository.

contribs/github-bot/client/client.go

gfanton · 2024-11-14T11:25:27Z

contribs/github-bot/main.go

nit: for consistency's sake among the project, I would prefer that you use ffcli here. You can check other main files in the project; most of them are using the internal command package, which is actually a fork of ffcli.

gfanton · 2024-11-14T12:00:01Z

.github/workflows/bot.yml

+  # This job creates a matrix of PR numbers based on the inputs from the various
+  # events that can trigger this workflow so that the process-pr job below can
+  # handle the parallel processing of the pull-requests
+  define-prs-matrix:


@aeddi some additional questions here:

Is there a scenario where one PR can block another PR? Or concurrency only happen when manually triggered?

How much time does a job usually take to complete? If one PR can block another PR and take 10 minutes (e.g., long process, network issue, ...) to complete, it might start to be a problem.

contribs/github-bot/comment.go

aeddi · 2024-11-14T16:26:44Z

About the PR opened from a fork : I remember we discussed it together, but I totally forgot to take it into account. The workaround is to use the pull_request_target trigger event to enforce the use of the workflow from the base branch instead of the version from the head branch of the PR.

In this context, GitHub allows the workflow to access to GitHub Actions secrets and also allows GITHUB_TOKEN to request for write permission.

I just tested it on the demo repo, and it works. I will make the correction, thanks for the reminder.

Edit : done fb598ee

aeddi self-assigned this Oct 28, 2024

aeddi marked this pull request as draft October 28, 2024 10:00

zivkovicmilos self-requested a review October 28, 2024 10:04

aeddi force-pushed the dev/aeddi/github-bot branch from 8da569e to 4a3e93d Compare November 4, 2024 10:12

aeddi force-pushed the dev/aeddi/github-bot branch from 7b63f89 to 1a34f18 Compare November 5, 2024 23:31

aeddi marked this pull request as ready for review November 6, 2024 00:51

aeddi force-pushed the dev/aeddi/github-bot branch from 1a34f18 to 7aa06b8 Compare November 6, 2024 00:55

GnoCheckBot mentioned this pull request Nov 6, 2024

Test PR GnoCheckBot/demo#1

Open

This was referenced Nov 6, 2024

PR label automation #3072

Open

Codify the PR review process #3073

Open

thehowl mentioned this pull request Nov 8, 2024

ci: disable codecov blocking PRs until it understands coverage #3090

Closed

aeddi force-pushed the dev/aeddi/github-bot branch 5 times, most recently from 3f419b8 to f85b439 Compare November 11, 2024 20:44

aeddi added 8 commits November 11, 2024 22:11

feat: add github bot

9bb6e18

typos

6524626

chore: move folder

7907007

fix: typos and comments

2c92197

style: typo fix lint

f173c9b

fix: small fixes + dry-run

26af26d

style: space out bash script in workflow

76a5402

chore: change package path

18c0399

aeddi added 7 commits November 11, 2024 22:11

docs: add a README and improve comments

9ac61e8

test: add units tests (wip)

cde2586

test: all req tested

3df9195

lint: try downgrading to a compatible go version

6d15715

lint: ignore lostcancel warning

30c7267

test: add branch req + fix loop closure

99256ed

test: improve coverage

78268ac

aeddi force-pushed the dev/aeddi/github-bot branch from f85b439 to 78268ac Compare November 11, 2024 21:12

zivkovicmilos requested a review from ajnavarro November 12, 2024 13:56

ajnavarro requested changes Nov 13, 2024

View reviewed changes

Kouteki added the in focus Core team is prioritizing this work label Nov 13, 2024

gfanton requested changes Nov 14, 2024

View reviewed changes

This was referenced Nov 14, 2024

Create bar GnoCheckBot/demo#6

Closed

Test GnoCheckBot/demo#7

Open

aeddi added 4 commits November 14, 2024 18:29

ci: change for pull request target

fb598ee

feat: add github client context cancelation

8d62467

refacto: remove useless else statement

66ce612

refacto: exit from main instead of handleCommentUpdate func

b12be4d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: add a github bot to support advanced PR review workflows #3037

ci: add a github bot to support advanced PR review workflows #3037

aeddi commented Oct 28, 2024 •

edited

Loading

codecov bot commented Oct 28, 2024 •

edited

Loading

thehowl commented Oct 30, 2024

ltzmaxwell commented Nov 5, 2024

aeddi commented Nov 5, 2024 •

edited

Loading

ltzmaxwell commented Nov 6, 2024

aeddi commented Nov 6, 2024

moul commented Nov 12, 2024

aeddi commented Nov 12, 2024 •

edited

Loading

ajnavarro left a comment

ajnavarro Nov 13, 2024

aeddi Nov 13, 2024

ajnavarro Nov 13, 2024

aeddi Nov 13, 2024

ajnavarro Nov 13, 2024

aeddi Nov 13, 2024

ajnavarro Nov 13, 2024

aeddi Nov 13, 2024

gfanton Nov 14, 2024

aeddi Nov 14, 2024

aeddi commented Nov 13, 2024 •

edited

Loading

gfanton left a comment •

edited

Loading

gfanton Nov 14, 2024

gfanton Nov 14, 2024

aeddi commented Nov 14, 2024 •

edited

Loading

ci: add a github bot to support advanced PR review workflows #3037

Are you sure you want to change the base?

ci: add a github bot to support advanced PR review workflows #3037

Conversation

aeddi commented Oct 28, 2024 • edited Loading

To do

codecov bot commented Oct 28, 2024 • edited Loading

Codecov Report

thehowl commented Oct 30, 2024

ltzmaxwell commented Nov 5, 2024

aeddi commented Nov 5, 2024 • edited Loading

ltzmaxwell commented Nov 6, 2024

aeddi commented Nov 6, 2024

moul commented Nov 12, 2024

aeddi commented Nov 12, 2024 • edited Loading

ajnavarro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeddi commented Nov 13, 2024 • edited Loading

gfanton left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeddi commented Nov 14, 2024 • edited Loading

aeddi commented Oct 28, 2024 •

edited

Loading

codecov bot commented Oct 28, 2024 •

edited

Loading

aeddi commented Nov 5, 2024 •

edited

Loading

aeddi commented Nov 12, 2024 •

edited

Loading

aeddi commented Nov 13, 2024 •

edited

Loading

gfanton left a comment •

edited

Loading

aeddi commented Nov 14, 2024 •

edited

Loading